gh-90562: Improve zero argument support for super() in dataclasses when slots=True#124692
gh-90562: Improve zero argument support for super() in dataclasses when slots=True#124692Bobronium wants to merge 18 commits intopython:mainfrom
super() in dataclasses when slots=True#124692Conversation
2e0ca01 to
3b65acf
Compare
Lib/dataclasses.py
Outdated
| return False | ||
|
|
||
|
|
||
| def _find_inner_functions(obj, _seen=None, _depth=0): |
There was a problem hiding this comment.
Why the leading underscores on _seen and _depth?
There was a problem hiding this comment.
This should indicate that these arguments are only used within the function (in recursive calls) and not outside of the function body.
There was a problem hiding this comment.
I think that the whole function is private, so it can only be used inside CPython by us :)
There was a problem hiding this comment.
Well, you've got a point. Removed the underscores.
Lib/dataclasses.py
Outdated
| _seen.add(id(obj)) | ||
|
|
||
| _depth += 1 | ||
| if _depth > 2: |
There was a problem hiding this comment.
Why are depth 1 and 2 special? What is this test (and _depth in general) trying to accomplish? Please add comments explaining what's going on here, it's not at all clear from the code.
There was a problem hiding this comment.
Added an explanation and few test cases for this behaviour. Caught two bugs in the process \o/
Lib/dataclasses.py
Outdated
| ): | ||
| # we don't want to inspect custom descriptors just yet | ||
| # there's still a chance we'll encounter a pure function | ||
| # or a property |
There was a problem hiding this comment.
Please add a comment explaining why encountering a pure function or property means we don't have to inspect custom descriptors.
There was a problem hiding this comment.
Hmm, there's already an explanation above:
Lines 1336 to 1339 in 3b65acf
Would you like me to add clarification for this case specifically?
There was a problem hiding this comment.
Ah, you're right. Maybe change the comment to "... fallback to inspecting custom descriptors if no pure function or property is found".
There was a problem hiding this comment.
Makes sense. I've added clarification in both places to be extra clear.
sobolevn
left a comment
There was a problem hiding this comment.
Great find, thanks a lot for your work! 👍
Lib/dataclasses.py
Outdated
| return False | ||
|
|
||
|
|
||
| def _find_inner_functions(obj, _seen=None, _depth=0): |
There was a problem hiding this comment.
I think that the whole function is private, so it can only be used inside CPython by us :)
Lib/dataclasses.py
Outdated
| return None | ||
|
|
||
| for attr in dir(obj): | ||
| value = getattr(obj, attr, None) |
There was a problem hiding this comment.
This case is always complex, since getattr can evaluate properties and other objects. There are two ways of hanling this case:
- Allow all exceptions to raise
- Hide all exceptions from user here
I am not quite sure which one is better here.
There was a problem hiding this comment.
Thanks for bringing this to my attention!
After some hacking, I've decided to go with the third option: removing any possibility of user defined code execution.
Co-authored-by: sobolevn <mail@sobolevn.me>
carljm
left a comment
There was a problem hiding this comment.
This looks OK to me; I'll let @ericvsmith give it final approval/merge.
I don't love that we've opened the door to an infinite fractal of complexity here; at some point it would probably be better to bite the bullet and introduce a __classcell__ attribute on classes that allows reliably finding this cell in O(1) time.
|
I agree that the complexity is unfortunate. |
This uses builtins.dir, which will trigger custom __getattibute__ if any, and will trigger __get__ on __dict__ descriptor.
Good idea. I've changed the implementation to use |
|
@carljm, friendly ping :) |
|
As I mentioned above, I would rather have @ericvsmith approve/merge this, since it was his review comments you addressed. |
|
I've been busy, but it's on my list of things to do. |
Added two tests for two cases that weren't covered in original PR: